-
Notifications
You must be signed in to change notification settings - Fork 1
Various code cleanup/minor bug fixes/readme improvemnets #67
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
6495095 to
101ac29
Compare
moeodeh3
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one big organizational note - Flutter/Dart supports a feature similar to Swift where you can define a single class across multiple files (via extension). If we can logically split things up instead of keeping everything in one large turnkey.dart file, that would be awesome
I'd recommend taking a look at our Swift SDK setup seen here - it’s a bit of a hassle to organize initially, but it improves readability a lot and I think this is worth it
| npm run build | ||
| npm start | ||
| ``` | ||
| You can find your `ORGANIZATION_ID` and `AUTH_PROXY_CONFIG_ID` from the [Turnkey Dashboard](https://app.turnkey.com). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should link to this guide: https://docs.turnkey.com/getting-started/quickstart#create-an-account
| ), | ||
| body: LoginScreen(), | ||
| ); | ||
| // We'll have the `onSessionSelected` callback navigate to the dashboard screen. You can also add another case here for AuthState.authenticated if you want to handle it directly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this feels a bit odd to me - I think we should just exhaust the switch statement and handle every AuthState, rather than relying on onSessionSelected here to take us to the dashboard screen
| } | ||
|
|
||
| final usingAuthProxy = (config.authProxyConfigId ?? '').isNotEmpty; | ||
| final usingAuthProxy = (config.authProxyConfigId ?? '').isNotEmpty && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this no longer tells us if they are just using authProxy, but instead tells us if they are using it and have the wallet-kit fetch enabled, so I think lets rename this
usingAuthProxy -> authProxyFetchEnabled
| final usingAuthProxy = (config.authProxyConfigId ?? '').isNotEmpty && | ||
| config.authConfig?.autoFetchWalletKitConfig == true; | ||
| if (usingAuthProxy) { | ||
| if (config.authConfig?.sessionExpirationSeconds != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
design choice: I know this was copied from JS, but in Swift we decided not to allow passing sessionExpirationSeconds in the config because:
- the console warning is misleading, this value is only used for Passkeys (and wallets in JS)
- it creates confusion for users who don’t have deep Turnkey infrastructure knowledge, since it’s unclear that it only applies to certain auth methods and is ignored for most
instead we removed it from the general config and require it to be passed directly into the relevant auth functions (e.g. loginWithPasskey())
this is one of the breaking changes I plan on making for js! So lets get ahead of it here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't think thats necessary, I think the warning is sufficient and it provides clarity on the frontend what your auth proxy config is if you ever need to access it in the app.
| : config.authConfig?.sessionExpirationSeconds ?? | ||
| AUTH_DEFAULT_EXPIRATION_SECONDS); | ||
|
|
||
| final otpAlphanumeric = proxyAuthConfig?.otpAlphanumeric ?? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay having otpAlphanumeric is useless I think in our mobile sdks?
In js we do this I think because we have UI components, and if someone wanted to use the authProxy with our UI components without having to fetch for wallet-kit config, they can
here that doesn't apple and I think there is literally no point of having this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking deeper here, we have a lot of stuff blindly copied over thats useless and cluttering our config. Can you copy what we do in swift seen here instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while you're at it, want to also rename:
masterConfig -> runtimeConfig
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The config includes everything for clarity, that way you don't have to query the auth proxy when you wan't to figure out what config it's running. It's also only clutter if you try to set it, which it specifically tells you not to...
|
|
||
| session = s; | ||
| _createClient( | ||
| createClient( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we also need to pass in authProxyId and authProxyUrl here so the client can make requests to the Auth Proxy. For example, an authenticated user might need to call the Auth Proxy when updating their email and keeping it verified - they’d use it to send and verify the OTP
can you check all createClient() calls and make sure they all have this passed in!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh nvm our createClient() helper already does this I think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pulls from masterConfig
| bool? invalidateExisting, | ||
| void Function(String oidcToken)? onSuccess, | ||
| String? publicKey, | ||
| void Function( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one thing to flag is our onSuccess() in js returns any, I don't think this matters though
|
|
||
| String toUtf8String(Uint8List bytes) => utf8.decode(bytes); | ||
|
|
||
| String bytesToHex(Uint8List bytes) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can remove this and use uint8ArrayToHexString() from turnkey_encoding
| const CreateP256UserParams({this.userName, this.apiKeyName}); | ||
| } | ||
|
|
||
| class PolicyWithId extends v1CreatePolicyIntentV3 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Policies have ids, its just in our fetchOrCreatePolicies() we basically have a v1Policy minus the created_at and updated_at timestamps, and we don't want to have to do another query to get those, so we just created another type
anyways I think we should call this just Policy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's to assuming this isn't breaking 🍻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
net new function so we are okay!
| /// [invalidateExisting] Optional flag to invalidate existing sessions when logging in or signing up. | ||
| /// [publicKey] Optional public key to use for the session. If null, a new key pair is generated. | ||
| /// [onSuccess] Optional callback function that receives the oidcToken, publicKey and providerName upon successful authentication, overrides default behavior. | ||
| Future<void> handleXOAuth({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we fix state validation here, we have state isn't acting as state. Lets do this for OAuth2 functions (X, Discord)
you can look at what we do here in Swift
…, made oauth state useful
…ed uneeded params from dev facing config
| ), | ||
| body: LoginScreen(), | ||
| ); | ||
| // We'll have the `onSessionSelected` callback navigate to the dashboard screen. You can also add another case here for AuthState.authenticated if you want to handle it directly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for this anymore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should be able to remove the onSessionSelected() from the example as well!
void onSessionSelected(Session session) {
if (isValidSession(session)) {
WidgetsBinding.instance.addPostFrameCallback((_) {
navigatorKey.currentState?.pushReplacement(
MaterialPageRoute(builder: (context) => const DashboardScreen()),
);
final ctx = navigatorKey.currentContext;
if (ctx != null) {
ScaffoldMessenger.of(ctx).showSnackBar(
const SnackBar(
content: Text('Logged in! Redirecting to the dashboard.'),
),
);
}
});
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nah that just shows a loading bar now when we are authenticated but we haven't heard from the onSessionSelected callback so we don't flash the login screen.
| /// [rpId] The Relying Party ID to use for Passkey authentication. If null, the value from the config's PasskeyStamperConfig will be used. | ||
| /// [overrideExisting] Whether to override the existing client instance with the newly created one. If true, all helper functions within the TurnkeyProvider will be using this client and thus, will be stamping using a passkey. Defaults to false. | ||
| /// Returns the newly created TurnkeyClient instance configured for Passkey stamping. | ||
| TurnkeyClient createPasskeyClient( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we move all the passkey stuff into its own extension as well 🙏
| } | ||
|
|
||
| // --- resolved methods ------------------------------------------------------ | ||
| final resolvedMethods = AuthMethods( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can remove this
Cleanup and various fixes for sdk_flutter and demo app.
New features:
signMessagemethod added. Signs a plaintext message using the specified wallet account. Automatically determines the payload encoding and hash function based on the wallet account's address format, unless explicitly overriddenautoFetchWalletKitConfigcan now be disabled in theTurnkeyProviderconfigPasskeyConfigis now configurable in theTurnkeyProviderconfig. This allows you to set anrpIdandrpNamefor all functions that need themcreateClientandcreatePasskeyClientfunctions are now exposedfetchOrCreateP256ApiKeyUsermethod added. Fetches an existing user by P-256 API key public key, or creates a new one if none exists.fetchOrCreatePoliciesmethod added. Fetches each requested policy if it exists, or creates it if it does not.Bug fixes / minor improvements:
authProxyBaseUrlis now properly defaultedsessionExpirationSecondsis now properly considered for passkey authhandleAppleOauthnow correctly uses Turnkey's OAuth Proxy url